refactor(SwingSet): minor tidying extracted from #9539 #9558
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
09b921c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://35a81364.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-tidy-v-v-admin.agoric-sdk.pages.dev |
|
BTW, I think #9539 (comment) is the right link to the discussion about why this was pulled out of the other PR.. the original link was to lines of code being changed, which went stale when this particular edit was removed from that branch. |
Huh? In what way is harden "turned off"? From what I can tell, willharden any return values from a message delivery, just before serializing the answer for syscall.resolve(). So technically this PR is a NOP just because the result of finishVatCreation() is promptly returned from createVat(), which is always invoked as E(vatAdminSvc).createVat(), i.e. as a message delivery. So the value is hardened a turn or two later, and in any case the recipient of the value is in a different vat (and their copy will be hardened by their marshal.unserialize).
We've done performance experiments where harden is replaced with a NOP, and we have test environments in which It's certainly best practice to harden before the The only significant consideration here is to have a clear policy on when/how we make changes to code for which we don't have a clear deployment scheduled. This change to vat-vat-admin is fine (it doesn't change externally-visible behavior). It will sit on A more interesting change, though (eg adding a new method, or relaxing some constraint), would require more attention. We should probably have a table somewhere of changes that have gone into trunk but are not yet deployed to the chain, so we can be deliberate about when+how they get deployed. And, we must be mindful in the meantime that trunk CI is testing against undeployed code, so e.g. a new vat that thinks it can use this new vat-vat-admin method, and passes in CI, must not get deployed to the chain before the vat-vat-admin upgrade. That's a bigger discussion, of course, so this note is mostly serving to convince ourselves that this particular PR does not need to wait for that bigger discussion. |
|
https://github.com/endojs/endo/blob/master/packages/init/unsafe-fast.js sets which causes the So all importers of run with const fakeHarden = arg => arg |
closes: #XXXX
refs: https://github.com/Agoric/agoric-sdk/pull/9539/files#r1648285867
Description
As @warner suggests at #9539 (comment) , I'm moving that change to this separate PR so we can decide separately when to merge it. It should be a pure refactor, since nothing should have been counting on the absence of the
hardenSecurity Considerations
hardening early is better for integrity, and will catch some integrity-violating bugs (property mutations) earlier. Almost certainly no difference in this case though, but good precedent for reenforce best practices.In fact, within the SwingSet kernel, this cannot have any effect on production under current configurations, where
hardenis turned off for SwingSet anyway. But at least we still have the option of turninghardenon when testing, in which case we still get the bug finding benefit.Finally, it is possible we will someday find we can afford to turn
hardenback on for SwingSet as a whole, in which case we get back this integrity protection for real.PLEASE establish the habit of
hardening literals before they escape whenever possible!Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
hardenin SwingSet could be turned on during testing, in which case thesehardencalls with detect more bugs.Upgrade Considerations
Why we pulled this out into a separate PR. See https://github.com/Agoric/agoric-sdk/pull/9539/files#r1648285867